Skip to content

refine chat text area focus#379

Merged
iceljc merged 2 commits into
SciSharp:mainfrom
iceljc:features/refine-chat-window
Sep 4, 2025
Merged

refine chat text area focus#379
iceljc merged 2 commits into
SciSharp:mainfrom
iceljc:features/refine-chat-window

Conversation

@iceljc
Copy link
Copy Markdown
Collaborator

@iceljc iceljc commented Sep 4, 2025 •
edited by qodo-code-review Bot
Loading

PR Type

Enhancement

Description

  • Add automatic focus to chat text area

  • Implement focus function with proper timing

  • Add ID prop to ChatTextArea component

  • Focus on component mount and editor load

Diagram Walkthrough

flowchart LR
  A["Chat Box Component"] --> B["Focus Function"]
  B --> C["Chat Text Area"]
  A --> D["onMount Event"]
  A --> E["loadEditor Reactive"]
  D --> B
  E --> B
  B --> F["textarea.focus()"]
Loading

File Walkthrough
Relevant files
Enhancement
chat-box.svelte
Implement automatic chat textarea focus                                   

src/routes/chat/[agentId]/[conversationId]/chat-box.svelte

  • Add focusChatTextArea() function with Promise-based focus logic
  • Call focus function on component mount and when editor loads
  • Pass id prop to ChatTextArea component
 +18/-0   
chat-text-area.svelte
Add ID prop to ChatTextArea                                                           

src/routes/chat/[agentId]/[conversationId]/chat-util/chat-text-area.svelte

  • Add id export prop for component identification
  • Apply id attribute to textarea element
 +4/-0     

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Sep 4, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Focus Timing

Focusing on every reactive change to loadEditor could cause unexpected scroll jumps or steal focus from users typing elsewhere; confirm this only triggers when appropriate (e.g., when the textarea is visible and not already focused).

 
$: {
	// const editor = lastBotMsg?.rich_content?.editor || '';
	loadTextEditor = true; // TEXT_EDITORS.includes(editor) || !Object.values(EditorType).includes(editor);
	loadEditor = !isSendingMsg && !isThinking && loadTextEditor && messageQueue.length === 0;
	if (loadEditor) {
		focusChatTextArea();
	}
}
Optional Prop Handling

The new id prop is applied directly; ensure uniqueness across multiple instances and consider defaulting or validating to avoid duplicate IDs.

 
<script>
	import { clickoutsideDirective } from "$lib/helpers/directives";
    import _ from "lodash";

    /** @type {string} */
    export let id;

    /** @type {string} */
    export let className = '';

    /** @type {string} */
Missing Null-Safety

document.getElementById('chat-textarea') assumes DOM availability; add optional chaining or type guard to avoid runtime errors if SSR or ID mismatch occurs and consider checking textarea instanceof HTMLElement before focus.

 
function focusChatTextArea() {
	return new Promise(resolve => {
		tick().then(() => {
			const textarea = document.getElementById('chat-textarea');
			if (textarea) {
				textarea.focus();
			}
			resolve('focused');
		});
	});
}

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Sep 4, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove invalid await usage

Using await inside a non-async onMount callback causes a syntax error. Remove
the await or make the callback async; since you don't use the result, simply
call the function without awaiting.

src/routes/chat/[agentId]/[conversationId]/chat-box.svelte [277]

-await focusChatTextArea();
+focusChatTextArea();
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a syntax error, as await is used inside a non-async onMount callback, which would cause the application to crash.

High
Guard DOM access in SSR

Accessing document during SSR will throw. Add a browser guard and return the
tick() promise directly to avoid an unnecessary Promise wrapper and prevent SSR
crashes.

src/routes/chat/[agentId]/[conversationId]/chat-box.svelte [284-294]

 function focusChatTextArea() {
-	return new Promise(resolve => {
-		tick().then(() => {
-			const textarea = document.getElementById('chat-textarea');
-			if (textarea) {
-				textarea.focus();
-			}
-			resolve('focused');
-		});
+	if (typeof document === 'undefined') {
+		return Promise.resolve('skipped');
+	}
+	return tick().then(() => {
+		const textarea = document.getElementById('chat-textarea');
+		if (textarea && typeof textarea.focus === 'function') {
+			textarea.focus();
+		}
+		return 'focused';
 	});
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that accessing document can cause a server-side rendering (SSR) crash and provides a proper guard, which is critical for SvelteKit applications.

High
High-level
Guard DOM focus; avoid IDs

Calling document.getElementById('chat-textarea') from a reactive block can
execute during SSR and crash, and a hard-coded global id risks collisions if
multiple chat boxes exist. Gate all focus logic behind onMount or a browser
check, and target the element via a bound ref (e.g., bind:this or a use:focus
action) instead of a global id to keep it SSR-safe and component-scoped.

Examples:

src/routes/chat/[agentId]/[conversationId]/chat-box.svelte [218-294] 
		if (loadEditor) {
			focusChatTextArea();
		}
	}

	$: {
		disableAction = !ADMIN_ROLES.includes(currentUser?.role || '') && currentUser?.id !== conversationUser?.id || !AgentExtensions.chatable(agent);
	}

	setContext('chat-window-context', {

 ... (clipped 67 lines)
src/routes/chat/[agentId]/[conversationId]/chat-util/chat-text-area.svelte [6-116] 
    export let id;

    /** @type {string} */
    export let className = '';

    /** @type {string} */
    export let text;

    /** @type {boolean} */
    export let loadUtils = false;

 ... (clipped 101 lines)

Solution Walkthrough:

Before:

// chat-box.svelte
$: if (loadEditor) {
    focusChatTextArea();
}

function focusChatTextArea() {
  return new Promise(resolve => {
    tick().then(() => {
      const textarea = document.getElementById('chat-textarea');
      if (textarea) {
        textarea.focus();
      }
    });
  });
}
...
<ChatTextArea id={'chat-textarea'} ... />

After:

// chat-box.svelte
import { browser } from '$app/environment';
let chatTextAreaComponent;

$: if (loadEditor && browser) {
  tick().then(() => chatTextAreaComponent?.focus());
}

onMount(async () => {
  await tick();
  chatTextAreaComponent?.focus();
});
...
<ChatTextArea bind:this={chatTextAreaComponent} ... />

// chat-text-area.svelte
let textareaElement;
export function focus() {
  textareaElement?.focus();
}
...
<textarea bind:this={textareaElement} ... />
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical server-side rendering (SSR) crash risk and a component reusability issue due to the use of a hard-coded ID, proposing a much more robust and idiomatic Svelte solution.

High
  • Update

@iceljc iceljc merged commit f62d7f2 into SciSharp:main Sep 4, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@iceljc